Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

produce source maps for middlewares #34409

Merged
merged 8 commits into from
Feb 24, 2022

Conversation

Schniz
Copy link
Contributor

@Schniz Schniz commented Feb 16, 2022

This PR will generate source maps for middlewares. It's not under any flag (should it be?)

Closes #34523

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@Schniz Schniz force-pushed the create-sourcemaps-for-middlewares branch from 6b760ce to 4758d92 Compare February 16, 2022 18:18
@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk
Copy link
Member

ijjk commented Feb 16, 2022

Failing test suites

Commit: 6bdf34c

yarn testheadless test/integration/gssp-ssr-change-reloading/test/index.test.js

  • GS(S)P Server-Side Change Reloading > should update page when getStaticProps is changed only
  • GS(S)P Server-Side Change Reloading > should show indicator when re-fetching data
  • GS(S)P Server-Side Change Reloading > should update page when getStaticPaths is changed only
  • GS(S)P Server-Side Change Reloading > should update page when getServerSideProps is changed only
Expand output

● GS(S)P Server-Side Change Reloading › should update page when getStaticProps is changed only

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  56 |
  57 |     const props = JSON.parse(await browser.elementByCss('#props').text())
> 58 |     expect(props.count).toBe(1)
     |                         ^
  59 |
  60 |     const page = new File(join(appDir, 'pages/gsp-blog/[post].js'))
  61 |     page.replace('count = 1', 'count = 2')

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:58:25)

● GS(S)P Server-Side Change Reloading › should show indicator when re-fetching data

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  82 |
  83 |     const props = JSON.parse(await browser.elementByCss('#props').text())
> 84 |     expect(props.count).toBe(1)
     |                         ^
  85 |
  86 |     const page = new File(join(appDir, 'pages/gsp-blog/[post].js'))
  87 |     page.replace('count = 1', 'count = 2')

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:84:25)

● GS(S)P Server-Side Change Reloading › should update page when getStaticPaths is changed only

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  108 |
  109 |     const props = JSON.parse(await browser.elementByCss('#props').text())
> 110 |     expect(props.count).toBe(1)
      |                         ^
  111 |
  112 |     const page = new File(join(appDir, 'pages/gsp-blog/[post].js'))
  113 |     page.replace('paths = 1', 'paths = 2')

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:110:25)

● GS(S)P Server-Side Change Reloading › should update page when getServerSideProps is changed only

TIMED OUT: 1

2

  474 |
  475 |   if (hardError) {
> 476 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content)
      |           ^
  477 |   }
  478 |   return false
  479 | }

  at Object.check (lib/next-test-utils.js:476:11)
  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:170:5)

Read more about building and testing Next.js in contributing.md.

@ijjk

This comment has been minimized.

@Schniz Schniz force-pushed the create-sourcemaps-for-middlewares branch from 6bdf34c to c2e3c96 Compare February 17, 2022 09:47
@ijjk

This comment has been minimized.

@@ -1277,6 +1277,16 @@ export default async function getBaseWebpackConfig(
].filter(Boolean),
},
plugins: [
// Produce source maps for middlewares
new webpack.SourceMapDevToolPlugin({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be super clear about this @sokra this does not enable sourcemap generation for all files and then discard only the ones in include right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably only be applied for production right?

Copy link
Member

@sokra sokra Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only generates SourceMaps for the included files, but it enables SourceMap generation in loaders for all modules, so that could affect performance...

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking this pr on perf concerns that need to be addressed.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@Schniz Schniz force-pushed the create-sourcemaps-for-middlewares branch from 27b7c80 to f4e5f1a Compare February 24, 2022 10:16
@Schniz Schniz force-pushed the create-sourcemaps-for-middlewares branch from f4e5f1a to dafe3ea Compare February 24, 2022 10:16
@ijjk

This comment has been minimized.

packages/next/build/webpack-config.ts Outdated Show resolved Hide resolved
@ijjk

This comment has been minimized.

sokra
sokra previously approved these changes Feb 24, 2022
const PLUGIN_NAME = 'NextJsMiddlewareSourceMapsPlugin'
compiler.hooks.compilation.tap(PLUGIN_NAME, (compilation) => {
compilation.hooks.buildModule.tap(PLUGIN_NAME, (module) => {
module.useSourceMap = module.layer === 'middleware'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to override other conditionals if you have sourcemaps enabled in a different way 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably needs

if(!module.useSourceMap) to make sure you don't disable other plugins enabling sourcemaps for certain modules.

@@ -1272,6 +1273,11 @@ export default async function getBaseWebpackConfig(
].filter(Boolean),
},
plugins: [
...(!isServer &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still applies in development where eval sourcemaps are already enabled. Is that intended?

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be changed, then it looks good: https://github.com/vercel/next.js/pull/34409/files#r813847032

@ijjk

This comment has been minimized.

@ijjk
Copy link
Member

ijjk commented Feb 24, 2022

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
buildDuration 18.9s 18.2s -698ms
buildDurationCached 7.2s 7.1s -116ms
nodeModulesSize 367 MB 367 MB ⚠️ +3.54 kB
Page Load Tests Overall increase ✓
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
/ failed reqs 0 0
/ total time (seconds) 3.895 3.675 -0.22
/ avg req/sec 641.93 680.22 +38.29
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 2 1.942 -0.06
/error-in-render avg req/sec 1249.82 1287.58 +37.76
Client Bundles (main, webpack)
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 27.8 kB 27.8 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.5 kB 71.5 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 194 B 194 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 326 B 326 B
dynamic-HASH.js gzip 2.57 kB 2.57 kB
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 919 B 919 B
image-HASH.js gzip 5.05 kB 5.05 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.26 kB 2.26 kB
routerDirect..HASH.js gzip 321 B 321 B
script-HASH.js gzip 383 B 383 B
withRouter-HASH.js gzip 318 B 318 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.7 kB 14.7 kB
Client Build Manifests
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
_buildManifest.js gzip 460 B 460 B
Overall change 460 B 460 B
Rendered Page Sizes
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
index.html gzip 531 B 531 B
link.html gzip 544 B 544 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
buildDuration 22.5s 22s -533ms
buildDurationCached 6.9s 7.3s ⚠️ +451ms
nodeModulesSize 367 MB 367 MB ⚠️ +3.54 kB
Page Load Tests Overall increase ✓
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
/ failed reqs 0 0
/ total time (seconds) 3.939 3.836 -0.1
/ avg req/sec 634.7 651.66 +16.96
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.974 1.967 -0.01
/error-in-render avg req/sec 1266.52 1270.67 +4.15
Client Bundles (main, webpack)
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.1 kB 42.1 kB
main-HASH.js gzip 27.8 kB 27.8 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.6 kB 71.6 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 180 B 180 B
amp-HASH.js gzip 305 B 305 B
css-HASH.js gzip 321 B 321 B
dynamic-HASH.js gzip 2.56 kB 2.56 kB
head-HASH.js gzip 342 B 342 B
hooks-HASH.js gzip 911 B 911 B
image-HASH.js gzip 5.08 kB 5.08 kB
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.28 kB 2.28 kB
routerDirect..HASH.js gzip 314 B 314 B
script-HASH.js gzip 375 B 375 B
withRouter-HASH.js gzip 309 B 309 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.7 kB 14.7 kB
Client Build Manifests
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary Schniz/next.js create-sourcemaps-for-middlewares Change
index.html gzip 534 B 534 B
link.html gzip 547 B 547 B
withRouter.html gzip 527 B 527 B
Overall change 1.61 kB 1.61 kB
Commit: a3910a8

@kodiakhq kodiakhq bot merged commit 1a0d149 into vercel:canary Feb 24, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate Source Maps for Middleware
4 participants